Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unit-test: the Last Reporter #2900

Closed
wants to merge 12 commits into from
Closed

Unit-test: the Last Reporter #2900

wants to merge 12 commits into from

Conversation

ScottFreeCode
Copy link
Contributor

Everything changed when the browser got coverage.

…eralls typically uses the Job ID, not the Build ID, so presumably it is designed to handle multiple jobs in the same build?
200 ms timeout was already causing flakes even before the coverage additions anyway; testing suggests that specific tests won't pass with instrumentation without increased time.
… by covering it in the `BUILDTMP/mocha.js` bundle
@ScottFreeCode ScottFreeCode added area: browser browser-specific qa area: reporters involving a specific reporter labels Jun 22, 2017
@ScottFreeCode
Copy link
Contributor Author

Depends on (built on top of) #2886

Also fixes #2893

Copy link
Contributor Author

@ScottFreeCode ScottFreeCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to look at failures in Sauce and figure out how to get compatibility with all browsers... In the meantime, here's a bit more info on some of the code.

runner.emit('test', rootSuite.suites[1].tests[1]);
rootSuite.suites[1].tests[1].pending = true;
rootSuite.suites[1].tests[1].duration = 0;
runner.emit('pending', rootSuite.suites[1].tests[1]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remind me to revisit this suppression of the non-query-parameters portion of the link in #2728, which attempts to make that opening portion work in all browsers.

runner.emit('test', rootSuite.suites[1].tests[1]);
rootSuite.suites[1].tests[1].pending = true;
rootSuite.suites[1].tests[1].duration = 0;
runner.emit('pending', rootSuite.suites[1].tests[1]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

runner.emit('test', rootSuite.suites[1].tests[1]);
rootSuite.suites[1].tests[1].pending = true;
rootSuite.suites[1].tests[1].duration = 0;
runner.emit('pending', rootSuite.suites[1].tests[1]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could have pulled in the real Runner, but then this would have been more like an integration test than a unit test... (I made a tradeoff to reuse the real Suite and Test objects mainly because I felt that the code would be too bloated if I tried to recreate them completely but too brittle if I tried to recreate just the bits and pieces that the HTML reporter currently happens to be relying upon.)


it('should show an error if the tag is missing', function () {
try {
// TODO: Figure out why it's ok to add an element to display this error but not ok to just add the missing element in the first place.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can feel free to remove this comment and replace it with a new GitHub issue. I just needed to jot it down while it was on my mind but didn't want to get sidetracked from the work at hand.


before(saveRealOutput); // so that the following afterEach is guaranteed to work

afterEach(function () { restoreOutput(); }); // WARNING: this is solely a fallback so *further* tests can be guaranteed to report correctly; these tests will not be reported correctly unless `restoreOutput` is called at the end of the test, before throwing any errors
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment here might not be true when running in Karma and not opening the debug page -- I don't know enough about how it works. I am certain that it is crucial for using the HTML reporter to report the HTML reporter tests, however (e.g. in a manual page such as the one added in this commit as well, or in Karma's debug usage of Mocha's HTML reporter). I'm considering updating the comment to clarify this. It's the explanation for the try-catching in all the tests, so I hope it's clear enough.

karma.conf.js Outdated
@@ -34,7 +34,9 @@ module.exports = function (config) {
// we use the BDD interface for all of the tests that
// aren't interface-specific.
'test/browser-fixtures/bdd.fixture.js',
'test/unit/*.spec.js'
'test/unit/*.spec.js',
'test/browser-unit/*.spec.js',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Planning on filling test/browser-unit with tests for lib/browser/*.js, especially any that don't just end up covered by extension of being used in other tested stuff.

karma.conf.js Outdated
'test/unit/*.spec.js'
'test/unit/*.spec.js',
'test/browser-unit/*.spec.js',
'test/browser-reporters/*.spec.js'
]),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could put the HTML reporter test in the browser unit folder (it's not like we're planning on adding multiple browser reporters in core), but this is more obvious by parallel of test/reporters.

@coveralls
Copy link

coveralls commented Jun 22, 2017

Coverage Status

Coverage increased (+4.2%) to 92.62% when pulling f50728a on add-browser-tests into 1f270cd on master.

@ScottFreeCode
Copy link
Contributor Author

I think the Snyk error may be because master has gotten an update to resolve the debug "vulnerability" (not that it's a real vulnerability, it's using the ms function that is not reportedly affected...), even though this PR doesn't introduce that "vulnerability" either.

@coveralls
Copy link

coveralls commented Jun 22, 2017

Coverage Status

Coverage increased (+4.3%) to 92.661% when pulling 5fcc1b5 on add-browser-tests into 1f270cd on master.

@mochajs mochajs deleted a comment from coveralls Jul 5, 2017
@mochajs mochajs deleted a comment from coveralls Jul 5, 2017
@mochajs mochajs deleted a comment from coveralls Jul 5, 2017
@mochajs mochajs deleted a comment from coveralls Jul 5, 2017
@mochajs mochajs deleted a comment from coveralls Jul 5, 2017
@mochajs mochajs deleted a comment from coveralls Jul 5, 2017
@mochajs mochajs deleted a comment from coveralls Jul 5, 2017
@mochajs mochajs deleted a comment from coveralls Jul 5, 2017
@mochajs mochajs deleted a comment from coveralls Jul 5, 2017
@ScottFreeCode
Copy link
Contributor Author

As this builds on #2886, it will need to be rebased from the old #2886 commits (last commit was 58e4b5d), at some point at least after we find ways to resolve the CI issues for #2886 (rebase onto the new #2886 in that case) and preferably after merging #2886 (rebase onto master in that case).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: browser browser-specific area: reporters involving a specific reporter area: repository tooling concerning ease of contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants